Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forge block timestamps in manual seal mode #76

Merged
merged 9 commits into from
Oct 29, 2020

Conversation

JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Oct 21, 2020

This PR introduces a FAKE timestamp inherent data provider that will always include timestamps that are one aura-slot-duration apart from one another. This allows us to use manual seal to author blocks quickly without violating timestamp assumptions made by either the Aura pallet or the Timestamp pallet.

This code was taken from polkadot-evm/frontier#170. When moonbeam updates to a Frontier version that includes that PR, we should re-evaluate whether it makes sense to keep this here.

Copy link
Collaborator

@crystalin crystalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also try to readd the ripemd one too ? It was already fixed I believe

@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Oct 22, 2020

I've tested this manually, and this change does allow manual seal to author blocks in quick succession as we expected. But it does not fix the failing tests in test-block.ts. When and why were those tests first skipped? They are working on the Frontier template, so it can't be about block production being throttled.

@crystalin
Copy link
Collaborator

@JoshOrndorff. My understanding is that those tests are fine with current implementatiuon of frontier + manual sealing.
However the change of Telmo + manual sealing => failing

@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Oct 22, 2020

Since manual seal was not the root cause of the failing tests, I no longer think it makes sense to merge this code in here. If we ultimately want it, we should wait to get it from Frontier where it has already been merged. I'll close this and re-focus on fixing the tests.

@JoshOrndorff
Copy link
Contributor Author

After discussion with @crystalin I understand we want this quick-authoring functionality in general, not just for one specific issue we were facing, so this should be re-opened.

I'll see if it needs updated and then get it ready for merge.

@JoshOrndorff JoshOrndorff reopened this Oct 26, 2020
@crystalin
Copy link
Collaborator

Do you think we could easily remove the Timestamp + Aura requirement in the Runtime ? and do you think it is worth trying to do so (to reduce the chance of having unexpected behaviors in the tests.)

@JoshOrndorff
Copy link
Contributor Author

To summarize our discussion in slack:

The main objective is to support both of the following in the standalone node:

  • A manual seal mode that can author blocks in quick succession
  • An aura mode that supports real standalone testnets

We cannot simply remove aura as it wouldn't support the second criteria.
We cannot remove aura using conditional compilation because cargo only supports one feature set per workspace.

I recommend we merge this PR as it supports both of the listed requirements and follows the same path that Frontier is following upstream cite.

@JoshOrndorff
Copy link
Contributor Author

Tests are failing atm because the actual fix for them is in #82.

Once that is merged, we can update this and CI will be green.

@JoshOrndorff JoshOrndorff merged commit c77c00e into master Oct 29, 2020
@JoshOrndorff JoshOrndorff deleted the joshy-forged-timestamps branch October 29, 2020 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants